Skip to content

Allow custom names for User-Defined Actions #7958

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Allow custom names for User-Defined Actions #7958

wants to merge 1 commit into from

Conversation

melihmutlu
Copy link
Contributor

@melihmutlu melihmutlu commented Apr 14, 2025 •
edited by fabriziomello
Loading

This commit allows custom names for Used-Defined Actions.

add_job() and alter_job() accept an additional "job_name" argument to modify the name of User-Defined Action. If "job_name" is not specified, it is names as default.

Closes #6991

@melihmutlu melihmutlu self-assigned this Apr 14, 2025
@CLAassistant
Copy link

CLAassistant commented Apr 14, 2025 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@melihmutlu melihmutlu force-pushed the allow_custom_uda_name branch from c049edd to 9470fe2 Compare April 14, 2025 14:00
@RobAtticus
Copy link
Member

Nice, so this would close #6991 then?

@fabriziomello fabriziomello added jobs Background Worker The background worker subsystem, including the scheduler labels Apr 14, 2025
@melihmutlu melihmutlu force-pushed the allow_custom_uda_name branch 9 times, most recently from 4cd8f17 to a9002db Compare April 15, 2025 14:59
mkindahl
mkindahl reviewed Apr 16, 2025
View reviewed changes
Copy link
Contributor

@mkindahl mkindahl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patch looks good, but I do not think it is necessary to add the job id if the user provides a job name.

It forces the user to think about this when reading job information using scripts or automation and also limits the usability for the user if they want to provide other information or information in different format as part of the job name.

@codecov Codecov
Copy link

codecov bot commented Apr 16, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 88.23529% with 2 lines in your changes missing coverage. Please review.

Project coverage is 82.18%. Comparing base (59f50f2) to head (b000625).
Report is 936 commits behind head on main.

Files with missing lines Patch % Lines
src/bgw/job.c 83.33% 0 Missing and 1 partial ⚠️
tsl/src/bgw_policy/job_api.c 90.90% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #7958      +/-   ##
==========================================
+ Coverage   80.06%   82.18%   +2.11%     
==========================================
  Files         190      253      +63     
  Lines       37181    46798    +9617     
  Branches     9450    11776    +2326     
==========================================
+ Hits        29770    38462    +8692     
- Misses       2997     3666     +669     
- Partials     4414     4670     +256

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@melihmutlu melihmutlu force-pushed the allow_custom_uda_name branch from a13d930 to 5cfbba4 Compare April 16, 2025 10:56
@philkra philkra added this to the v2.20.0 milestone Apr 16, 2025
@melihmutlu melihmutlu force-pushed the allow_custom_uda_name branch 2 times, most recently from 64cceae to 747ab7f Compare April 16, 2025 11:30
@svenklemm
Copy link
Member

Shouldn't the datatype in the API be text since name is for sql identifier which this is not.

@melihmutlu
Copy link
Contributor Author

Shouldn't the datatype in the API be text since name is for sql identifier which this is not.

application_name is name, and job names go into application_name. That's why I decided to go with name instead of text.

Another reason was thatname has a limit of 63 bytes, so it would help to have names with reasonable lengths. But now I realize that a format like "<job_name> [<job_id>]" can easily exceed that limit. Even without appending the job ID, users could provide long job names that get truncated to 63 bytes, potentially resulting in different jobs ending up with the same name.

Maybe we should have a limit on the length of job name, possibly a bit shorter than 63, to be safe. Any thoughts?

@melihmutlu melihmutlu force-pushed the allow_custom_uda_name branch 3 times, most recently from 91d7787 to 6c33371 Compare April 21, 2025 13:50
@melihmutlu melihmutlu force-pushed the allow_custom_uda_name branch 3 times, most recently from fb66024 to 423fad7 Compare April 22, 2025 07:39
@melihmutlu melihmutlu force-pushed the allow_custom_uda_name branch 3 times, most recently from 659b009 to abcd731 Compare April 29, 2025 15:55
@melihmutlu melihmutlu force-pushed the allow_custom_uda_name branch 2 times, most recently from 277e4fa to f73c309 Compare April 30, 2025 14:14
@melihmutlu
Allow custom names for User-Defined Actions
b000625 
This commit allows custom names for Used-Defined Actions.

add_job() and alter_job() accept an additional "job_name" argument to
modify the name of User-Defined Action. If "job_name" is not specified,
it is names as default.
@melihmutlu melihmutlu force-pushed the allow_custom_uda_name branch from f73c309 to b000625 Compare April 30, 2025 14:19
fabriziomello
fabriziomello reviewed Apr 30, 2025
View reviewed changes
tsl/test/sql/bgw_custom.sql

\c :TEST_DBNAME :ROLE_SUPERUSER

\set ON_ERROR_STOP 0
-- test bad input
SELECT alter_job(NULL, if_exists => false);
SELECT alter_job(-1, if_exists => false);
SELECT alter_job(1001, job_name => 'this_is_a_really_really_really_long_application_name_to_overflow');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think one test that we can add here is manually changing the job_id (update the job table) let's say to 2147483647 (max for int4) and then try to alter the job and make sure will work if the name fit with big job_id and fail if the name is bigger than the NAMEDATALEN.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabriziomello fabriziomello fabriziomello left review comments

@RobAtticus RobAtticus RobAtticus left review comments

@mkindahl mkindahl mkindahl approved these changes

Assignees

@melihmutlu melihmutlu

Labels
Background Worker The background worker subsystem, including the scheduler jobs
Projects
None yet
Milestone
v2.20.0
Development

Successfully merging this pull request may close these issues.

[Enhancement]: Allow specifying application_name for custom jobs
7 participants
@melihmutlu @CLAassistant @RobAtticus @svenklemm @fabriziomello @mkindahl @philkra